Skip to content

[grid] Forward file upload/download for Kubernetes, Docker and relay sessions - #17914

Merged
VietND96 merged 3 commits into
trunkfrom
claude/dynamic-grid-kubernetes-issue-3msnjn
Aug 13, 2026
Merged

[grid] Forward file upload/download for Kubernetes, Docker and relay sessions#17914
VietND96 merged 3 commits into
trunkfrom
claude/dynamic-grid-kubernetes-issue-3msnjn

Conversation

@VietND96

@VietND96 VietND96 commented Aug 13, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

Fixes SeleniumHQ/docker-selenium#3206 (remote file upload fails on the Kubernetes Dynamic Grid).

💥 What does this PR do?

On the Kubernetes Dynamic Grid (node-kubernetes), LocalFileDetector-based remote file uploads fail with File not found. The uploaded file lands on the Node Pod, but sendKeys runs in a separate browser Job Pod that cannot see it.

The root cause is an asymmetry in LocalNode: uploadFile() (and downloadFile()) only forward the command to the downstream session when it is a DockerSession. Docker Dynamic Grid works because the command is forwarded into the browser container; Kubernetes was never forwarded, so the file was written to the Node's own filesystem instead of the browser Pod's.

This PR forwards file upload and download commands to the session whenever the browser runs in a separate environment from the Node:

  • Docker container (already worked — behavior preserved)
  • Kubernetes Job Pod (the reported bug)
  • Relay target — an external endpoint (cloud provider, Appium device farm, another Selenium) that likewise does not share the relay Node's filesystem

Static Grid Nodes (browser in the same Pod/process as the Node) are unchanged and continue to be served from the Node's local filesystem.

🔧 Implementation Notes

The obvious fix — adding || slot.getSession() instanceof KubernetesSession next to the existing DockerSession check — was deliberately avoided. The kubernetes package pulls in the io.fabric8:kubernetes-client dependency, which is intentionally kept as an optional runtime extension (loaded via --ext; note fabric8-compile-only with neverlink = True and the kubernetes-embedded target). Making the core local package depend on kubernetes at compile time would drag fabric8 into core and break that extension model.

Instead this introduces a small capability method on the shared ActiveSession interface (which local, docker, kubernetes, and relay already depend on):

default boolean isRemoteFileSystem() {
  return false;
}
  • DockerSession, KubernetesSession, and the relay session (RelaySessionFactory) override/report it as true.
  • LocalNode.uploadFile() / downloadFile() forward the command when the session reports a remote filesystem.
  • Plain sessions keep the default false and are handled locally.

This removes the Docker-specific coupling from LocalNode, and fixes downloadFile() too, which had the identical Docker-only asymmetry (managed downloads / se/files would also fail on Kubernetes — not mentioned in the issue, but broken the same way).

Note on relay: relay targets are heterogeneous, and some (e.g. Appium device farms) use their own file-upload mechanism rather than the Grid /se/file endpoint. Forwarding is the correct default for a browser that does not share the Node's filesystem; if a specific relay target does not implement /se/file, that is a target-side concern. Happy to gate relay forwarding behind a capability/flag instead if maintainers prefer.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: The ActiveSession.isRemoteFileSystem() capability method, the DockerSession/KubernetesSession/relay overrides, the LocalNode upload/download changes, and the LocalNodeTest / RelaySessionFactoryTest regression tests.
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

The full build/tests could not be run in the authoring environment (the sandbox's egress policy blocks the Bazel binary download), so CI is the first place these targets run. The change is small and self-contained; the added tests exercise both the forwarded (remote-filesystem) and local (shared-filesystem) paths for upload and download, and assert that a relay session reports a remote filesystem.

🔄 Types of changes

  • Bug fix (backwards compatible)

Remote file upload (LocalFileDetector) failed on the Kubernetes Dynamic
Grid: LocalNode.uploadFile()/downloadFile() only forwarded the command to
the downstream session for DockerSession, so on node-kubernetes the file
was written to the Node Pod while sendKeys ran in a separate browser Job
Pod that could not see it.

Rather than adding an `instanceof KubernetesSession` check in LocalNode
(which would drag the optional fabric8 kubernetes-client dependency into
the core `local` package, breaking the --ext extension model), introduce a
capability method on ActiveSession:

  default boolean isRemoteFileSystem() { return false; }

DockerSession and KubernetesSession both override it to return true, and
LocalNode forwards upload/download whenever the session reports a remote
filesystem. Plain (static Grid) sessions keep the default and continue to
be handled on the Node's local filesystem.

This also fixes downloadFile(), which had the same Docker-only asymmetry
(managed downloads / se/files would likewise fail on Kubernetes).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RszyJNdSaBmaSRM5i1c3A
@qodo-code-review

Copy link
Copy Markdown
Contributor

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings labels Aug 13, 2026
A relay session forwards to an external endpoint (a cloud provider, an
Appium device farm, another Selenium) whose browser does not share the
relay Node's filesystem — the same situation as Docker/Kubernetes. Have
the relay-produced session report isRemoteFileSystem() = true so LocalNode
forwards file upload/download commands to the relay target instead of
writing them to the relay Node's own filesystem.

Also refreshes the surrounding comments/javadoc to mention relay.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RszyJNdSaBmaSRM5i1c3A
@VietND96 VietND96 changed the title [grid] Forward file upload/download for Kubernetes Dynamic Grid sessions [grid] Forward file upload/download for Kubernetes, Docker and relay sessions Aug 13, 2026
Formatting-only: ran google-java-format 1.36.1 (the repo-pinned version) on
the changed files. Fixes javadoc line-fill in ActiveSession and import
ordering / a call-site line-join in LocalNodeTest. No behavior change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RszyJNdSaBmaSRM5i1c3A
@VietND96
VietND96 merged commit aa36b38 into trunk Aug 13, 2026
47 checks passed
@VietND96
VietND96 deleted the claude/dynamic-grid-kubernetes-issue-3msnjn branch August 13, 2026 05:06
VietND96 pushed a commit to NDViet/docker-selenium that referenced this pull request Aug 13, 2026
Enable managed downloads on the desktop relay path (NodeFirefox relaying into
the standalone browser) so the upload and download tests actually exercise the
relay session file-operation forwarding from SeleniumHQ/selenium#17914.

Previously the relay test disabled managed downloads, so test_download_file
returned early and the relay download path was never verified. Now the relay
target standalone runs with managed downloads enabled and the client requests
downloads, so get_downloadable_files/downloadFile are forwarded through the
relay node to the standalone browser, and LocalFileDetector uploads are
forwarded the same way. This confirms the relay session reports a remote
filesystem.

Kept off for the Android emulator relay, where managed downloads and HTML file
upload via LocalFileDetector do not apply.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxGB7ifcYFULc8hDiwPfUf
VietND96 pushed a commit to NDViet/docker-selenium that referenced this pull request Aug 13, 2026
…rified

Turn on managed downloads for the Dynamic Grid deployments that spawn the
browser away from the Node, so the upload/download suite verifies the remote
file forwarding from SeleniumHQ/selenium#17914 instead of skipping it.

- Docker Dynamic Grid: default SELENIUM_ENABLE_MANAGED_DOWNLOADS to true in the
  test_node_docker target. It previously defaulted to false and only checked the
  legacy shared download volume; now the managed download API is retrieved
  through the DockerSession, exercising the forwarding path.
- Kubernetes Dynamic Grid: enable managed downloads on the standalone-kubernetes
  and node-kubernetes deployments and default the test flag to true. The Node
  forwards downloadFile to the browser Job Pod, so managed downloads are now
  retrievable; the outdated comment saying they are not is updated.

Enabling it on the Node mirrors the node-docker setup, where the dynamic-grid
Node handles retrieval for the browsers it spawns without extra per-browser
configuration.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxGB7ifcYFULc8hDiwPfUf
VietND96 pushed a commit to NDViet/docker-selenium that referenced this pull request Aug 13, 2026
…ents

CI failed on the Relay and Kubernetes Dynamic Grid lanes after enabling remote
downloads: those sessions only forward file upload/download to the browser with
SeleniumHQ/selenium#17914 (Selenium 4.48.0). On the current release base,
enabling managed downloads there makes get_downloadable_files time out and the
LocalFileDetector upload land where the browser cannot see it.

Gate the remote verification behind TEST_UPLOAD_DOWNLOAD_REMOTE (default true)
and turn it off for the Relay and Kubernetes harnesses until the Grid base
includes the fix. Docker Dynamic Grid and standalone keep verifying it, since
DockerSession forwards on every Selenium version. Also revert the managed
downloads enablement for the Relay standalone target and the Kubernetes Node
deployments so those lanes return to their known-good state.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxGB7ifcYFULc8hDiwPfUf
VietND96 pushed a commit to NDViet/docker-selenium that referenced this pull request Aug 13, 2026
…atch

The Relay lane failed with managed downloads on because only the relay target
(standalone) and the client had it enabled, not the relay Node itself. With the
Node's slot not advertising se:downloadsEnabled, a session created with
enable_downloads had no matching slot and failed, taking the whole lane down.

Enable managed downloads on node-relay-standalone too, so its slot advertises
se:downloadsEnabled, the request matches, and the Node forwards the file
operations to the standalone browser it relays into (SeleniumHQ/selenium#17914).

Also drop the TEST_UPLOAD_DOWNLOAD_REMOTE version gate added earlier: CI builds
on the Selenium nightly, which already carries the fix, so the remote
upload/download verification runs on Relay and Kubernetes as intended.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VxGB7ifcYFULc8hDiwPfUf
This was referenced Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Remote file upload fails on Kubernetes Dynamic Grid (node-kubernetes) — file written on Node pod, sendKeys runs in browser Job pod

2 participants